Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a Map for createTransformer memoization #331

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

peterm-canva
Copy link

@peterm-canva peterm-canva commented Aug 19, 2024

createTransformer() memoizes object transformations by creating a key for each possible input of type A, and then stores a mapping of that key to an object of type B in the context variable views. The key is stored on each A as a non-enumerable property named $transformId, so that each object can be looked up in the views cache later.

If we change views to be a Map instead of a plain object, we can get rid of the need for the IDs completely.

With this new impl, we still treat string and number inputs by value, and we still treat object inputs by identity. This comes down to the hashing semantics of Map.

This saves a small amount of memory in the unique memo IDs that were created for each input, and also some memory on each input object, which was modified to add the $transformId property. This may also have a performance benefit, because we aren't modifying the hidden shape of the inputs any more.

@peterm-canva peterm-canva marked this pull request as ready for review August 19, 2024 06:28
@peterm-canva
Copy link
Author

@taj-p PTAL

Copy link
Contributor

@taj-p taj-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I like how this prevents mutating the object, prevents the creation of another map, conserves memory, and uses a Map instead of a dynamic object.

@peterm-canva
Copy link
Author

With this approach, we are retaining the input objects in the cache Map directly. With the previous approach we only kept the string IDs in the cache object. I don't think this is an issue, because the input objects are already retained by the computed() closure stored in the cache?

@peterm-canva
Copy link
Author

@mweststrate PTAL when you have some time, thanks! 🙏

@peterm-canva
Copy link
Author

@urugator if you have thoughts on this

@urugator
Copy link
Contributor

urugator commented Sep 20, 2024

LGTM
I assume Map wasn't available at that time, therefore the object. Dunno if the depedency on Map should be a concern now. Guess not.
Seems like memoizationId:${++memoizationId} was actually meant to be object:${++memoizationId}.
It's a shame a debug name is generated regardless of env. I suspect you always provide custom debugNameGenerator?
Btw parameters documentation could use some work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants